Lint with mypy on CI - #219
Conversation
|
@nihalxkumar You modified some of these files in your PR, I think that it is easier:
Rather than fixing those errors in a separate PR, as that might cause some merge conflicts. |
|
I ran the mypy check on my branch. It reports the same 34 errors already present on main, with no new errors from my changes |
Bodies of untyped functions are not checked, so you can add return type to functions you added to make mypy lint them. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
==========================================
- Coverage 71.42% 71.32% -0.11%
==========================================
Files 12 12
Lines 1344 1381 +37
==========================================
+ Hits 960 985 +25
- Misses 384 396 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🙋 My patch for 213: https://gist.github.com/nihalxkumar/b5d76eb71ba8ebaa45ac90af08d6e9b2 |
I was trying to circumvent a mypy error that entrypoint had two types, |
|
|
||
| def add_new_download( | ||
| self, _user_data, description: str, total_to_download: float | ||
| self, _user_data, description: str, total_to_download: int |
There was a problem hiding this comment.
This (and other changes to function signatures in this class) is wrong - the function is called by libdnf and according to its documentation it uses float here:
| add_new_download(self, user_data, description, total_to_download)
| Notify the client that a new download has been created.
| :type user_data: void
| :param user_data: User data entered together with url/package to download.
| :type description: string
| :param description: The message describing new download (url/packagename).
| :type total_to_download: float
| :param total_to_download: Total number of bytes to download.
| :rtype: void
| :return: Associated user data for new download.
There was a problem hiding this comment.
Thanks. Was this automatic detection somehow that I could have done via editor or some other tool? To fix, I looked at help(libdnf5.repo.DownloadCallbacks.add_new_download).
There was a problem hiding this comment.
Nope, I just checked type changes in this PR that looked suspicious to me.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026080104-devel&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026050504-devel&flavor=update
Failed tests12 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/176874#dependencies 26 fixed
Unstable testsDetails
Performance TestsPerformance degradation:28 performance degradations
Remaining performance tests:83 tests
|
| # silent is equivalent to quiet for dom0-update | ||
| entrypoint.append("--silent") | ||
| entrypoint_cmd.append("--silent") | ||
| entrypoint = " ".join(entrypoint_cmd) |
There was a problem hiding this comment.
Here, you changed what is actually set to entrypoint. It used to be a list, now you set str joined from that list. Look at QubeConnection.run_entrypoint() - it behaves differently depending on type, and I'm pretty sure it won't work this way (it will try to run python3 sudo qubesd-dom0-update ...).
There was a problem hiding this comment.
This is still an issue in the latest version (at fe9f5a2)
|
As apparently I was doing bad a type hint, I took this opportunity to train and do it on vmupdate completely (except tests). I did not enjoy it at all, but I think I did better this time. |
|
And also CI is fully red (pylint, tests, mypy) |
|
Had issues annotating |
7d2429c to
033c14e
Compare
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Besides specific comments, is it a good idea to add this rather big PR, before #213 lands? Or is @nihalxkumar okay with potentially painful rebase?
| self.desc = desc | ||
| self.progress = 0 | ||
| self.desc: str = desc | ||
| self.n: float | None = 0.0 |
There was a problem hiding this comment.
Why n (it's really not more descriptive than progress), and also why pylint doesn't complain about too short attribute name?
There was a problem hiding this comment.
Oh, I see why, pylintrc has disable=invalid-name ...
There was a problem hiding this comment.
Why
n(it's really not more descriptive thanprogress)
In the commit message, I wrote this:
The progress bar variable changed from "progress" to "n" as that is what "tqdm" has.
The type hint was complaining that tqdm doesn't have progress attribute, and on my tests, it can't be set to None. I preferred using the same attribute name, although a very short single letter. If you prefer the long name, I will have to make sure that all writes to .progress attribute are made to .n if terminal bar is tqdm.
| qube_bar = progress_bar.progress_bars[qube.name] | ||
| progress = None | ||
| if isinstance(qube_bar, tqdm): | ||
| progress = 0.0 |
There was a problem hiding this comment.
Why, or more specifically - why here? Changing behavior based on what progress class is used (outside of that class) looks like a footgun, in case somebody wants to implement different progress frontend.
There was a problem hiding this comment.
answered on the comment above why. About why here, no specific reason, it's just the only place where it was needed. I will see about making a method to avoid accessing the attribute directly, and setting to a value is that not valid depending on the terminal bar.
There was a problem hiding this comment.
The code comment just above suggests the None value was important. Is it not for tqdm?
There was a problem hiding this comment.
tqdm doesn't accept None as a value:
>>> import tqdm
>>> help(tqdm.tqdm)
>>> t = tqdm.tqdm()
0it [00:00, ?it/s]>>>
>>> vars(t)
{'iterable': None, 'desc': '', 'total': None, 'leave': True, 'fp': <tqdm.utils.DisableOnWriteError object at 0x7c918c388050>, 'ncols': 120, 'nrows': 19, 'mininterval': 0.1, 'maxinterval': 10.0, 'miniters': 0, 'dynamic_miniters': True, 'ascii': False, 'disable': False, 'unit': 'it', 'unit_scale': False, 'unit_divisor': 1000, 'initial': 0, 'lock_args': None, 'delay': 0.0, 'gui': False, 'dynamic_ncols': False, 'smoothing': 0.3, '_ema_dn': <tqdm.std.EMA object at 0x7c918c3881a0>, '_ema_dt': <tqdm.std.EMA object at 0x7c918c47b110>, '_ema_miniters': <tqdm.std.EMA object at 0x7c918c47b250>, 'bar_format': None, 'postfix': None, 'colour': None, '_time': <built-in function time>, 'last_print_n': 0, 'n': 0, 'pos': 0, 'sp': <function tqdm.status_printer.<locals>.print_status at 0x7c918c382200>, 'last_print_t': 1785770639.7390816, 'start_t': 1785770639.7390816}
>>> t.total = 300
>>> t.n = 50
>>> t.display()
17%|█████████████▋ | 50/300 [00:31<02:35, 1.61it/s]True
>>> 50 / 300
0.16666666666666666
>>> t.n = None
>>> t.display()
Traceback (most recent call last):
File "<python-input-11>", line 1, in <module>
t.display()
~~~~~~~~~^^
File "/usr/lib/python3.13/site-packages/tqdm/std.py", line 1495, in display
self.sp(self.__str__() if msg is None else msg)
~~~~~~~~~~~~^^
File "/usr/lib/python3.13/site-packages/tqdm/std.py", line 1151, in __str__
return self.format_meter(**self.format_dict)
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.13/site-packages/tqdm/std.py", line 534, in format_meter
if total and n >= (total + 0.5): # allow float imprecision (#849)
^^^^^^^^^^^^^^^^^^
TypeError: '>=' not supported between instances of 'NoneType' and 'float'There was a problem hiding this comment.
But this assumes tqdm.n is what needs to be set. This is absolutely not clear for me from reading the current code. tqdm's documentation never mentions this attribute explicitly, probably changing it behind its backs is a wrong idea (looks like it's supposed to be done via tqdm.update() method, which is called in MultipleUpdateMultipleProgressBar._update()).
There was a problem hiding this comment.
Thanks, this helped me notice a missing method, reset().
fb78641 to
e333b7a
Compare
Added "reset()" method to SimpleTerminalbar, compatible with tqdm.
I tested merging #213 on top of this PR, there was a conflict on two files, two conflicts per file. All were short and easy to solve. I tested rebase, the conflicts were spread across 3 commits. |
5 participants
I'd like mypy to run on #213.